Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

error when missing tpv and platform present #3691

Merged
merged 13 commits into from
Oct 21, 2020
Merged

error when missing tpv and platform present #3691

merged 13 commits into from
Oct 21, 2020

Conversation

zkat
Copy link
Contributor

@zkat zkat commented Sep 28, 2020 •
edited
Loading

Bug

Fixes: NuGet/Home#9441
Regression: No

Fix

This makes pack and restore fail if there's a framework with a platform but without a

Testing/Validation

Tests Added: Yes
Validation:
I manually checked it works for dependencygroups

@zkat zkat force-pushed the dev-zkat-net5-missing-tpv-error branch from d5a775b to 5c7a7ef Compare September 28, 2020 22:29
@zkat zkat force-pushed the dev-zkat-net5-missing-tpv-error branch from 5c7a7ef to e5206dd Compare October 12, 2020 21:30
@nkolev92 nkolev92 mentioned this pull request Oct 14, 2020
Closed
@zkat zkat force-pushed the dev-zkat-net5-missing-tpv-error branch from e5206dd to dbd5e0f Compare October 16, 2020 17:12
@zkat zkat marked this pull request as ready for review October 16, 2020 17:13
@zkat zkat requested a review from a team as a code owner October 16, 2020 17:13
dominoFire
dominoFire reviewed Oct 16, 2020
View reviewed changes
Copy link
Contributor

@dominoFire dominoFire left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting approach. I left some comments on Validation Rule.

src/NuGet.Core/NuGet.Commands/Strings.resx Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Packaging/Rules/TargetFrameworkMissingPlatformVersionRule.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Packaging/Rules/TargetFrameworkMissingPlatformVersionRule.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Packaging/Rules/TargetFrameworkMissingPlatformVersionRule.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Packaging/Strings.resx Outdated Show resolved Hide resolved
@zkat
Copy link
Contributor Author

zkat commented Oct 19, 2020

@nkolev92 is this what you meant?

@nkolev92
Copy link
Member

@zkat

Yep, looks like the correct place for the validation.
I'll do an in depth review tomorrow.

@zkat
Copy link
Contributor Author

zkat commented Oct 20, 2020

Tests are pushed. Providing everything passes, this is all the code to review, I think.

nkolev92
nkolev92 approved these changes Oct 21, 2020
View reviewed changes
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👏 👏 👏 👏 👏

I think we can add a few more tests, but looks good beyond that.

test/NuGet.Core.Tests/NuGet.Packaging.Test/PackageBuilderTest.cs Show resolved Hide resolved
@zkat zkat merged commit 11c2bcb into dev Oct 21, 2020
@zkat zkat deleted the dev-zkat-net5-missing-tpv-error branch October 21, 2020 22:08
@zkat zkat mentioned this pull request Nov 5, 2020
Merged
@mayerj mayerj mentioned this pull request Mar 18, 2021
Closed
@zkat zkat mentioned this pull request Mar 30, 2021
Merged
8 tasks
@nkolev92 nkolev92 mentioned this pull request May 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkolev92 nkolev92 nkolev92 approved these changes

@dominoFire dominoFire Awaiting requested review from dominoFire

@erdembayar erdembayar Awaiting requested review from erdembayar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

net5 tfm: produce error when missing TPV
4 participants
@zkat @nkolev92 @dominoFire @erdembayar